Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Header generation refactoring #2436

Draft
wants to merge 58 commits into
base: master
Choose a base branch
from
Draft

Header generation refactoring #2436

wants to merge 58 commits into from

Conversation

ATorrise
Copy link
Contributor

@ATorrise ATorrise commented Feb 14, 2025

This refactoring effort centralizes and standardizes header generation across the Files SDK and CLI, consolidating all header logic into a single utility class—ZosFilesHeaders—and replacing duplicate header creation code across multiple methods.

The new implementation leverages a unified headerMap to manage header generation functions, ensuring that headers like responseTimeout, etag, and others are applied consistently in all ZosFiles REST calls.

  • Header Generation now:

    • Requires:
      • the request options
    • Optionally takes:
      • dataLength parameter to add a Content-Length header when needed.
      • ZosFilesContext enum that guides which Content-Type headers are applied for certain methods. These enum values include:
        • USS_MULTIPLE
          • Forces a general content-type header ("Content-Type": "application/json") when multiple USS files.
        • ZFS& LIST
          • No content-type headers are applied.
        • UPLOAD
          • Applies the content-type header ("X-IBM-Data-Type": encoding ). And will pass Content-Type: application/octet-stream for binary data.
        • DOWNLOAD
          • applies Content-Type header.
        • No context (default)
          • applies the default content-type header: {"X-IBM-Data-Type": "text"}
    ZosFilesHeaders.generateHeaders<T>({
      options,
      context,
      dataLength,
    }: {
      options: T;
      context?: ZosFilesContext;
      dataLength?: number | string;
    }): IHeaderContent[]

Because of this centralization, organization, and newly-provided, clear header expectations, any future header changes or additions require modifications in only one location. Creating headers can now be done with a single function call without much thought required in future development of the methods within ZosFiles SDK.

@ATorrise ATorrise requested review from zFernand0, jace-roell and awharn and removed request for zFernand0 and jace-roell February 14, 2025 21:58
Copy link

github-actions bot commented Feb 14, 2025

📅 Suggested merge-by date: 3/5/2025

Signed-off-by: Amber <[email protected]>
Signed-off-by: Amber <[email protected]>
Copy link

codecov bot commented Feb 14, 2025

Codecov Report

Attention: Patch coverage is 97.39583% with 5 lines in your changes missing coverage. Please review.

Project coverage is 91.45%. Comparing base (26d6572) to head (8fa47b2).
Report is 8 commits behind head on master.

Files with missing lines Patch % Lines
packages/zosfiles/src/utils/ZosFilesHeaders.ts 95.96% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2436      +/-   ##
==========================================
- Coverage   91.50%   91.45%   -0.06%     
==========================================
  Files         641      642       +1     
  Lines       18419    18423       +4     
  Branches     3963     3872      -91     
==========================================
- Hits        16855    16848       -7     
- Misses       1562     1573      +11     
  Partials        2        2              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Signed-off-by: Amber Torrise <[email protected]>
ATorrise and others added 15 commits March 5, 2025 14:00
Signed-off-by: Amber <[email protected]>
Signed-off-by: Amber <[email protected]>
Signed-off-by: Amber <[email protected]>
Signed-off-by: Amber <[email protected]>
Signed-off-by: Amber <[email protected]>
Signed-off-by: Amber <[email protected]>
Signed-off-by: Amber <[email protected]>
Signed-off-by: Amber <[email protected]>
Signed-off-by: Fernando Rijo Cedeno <[email protected]>
Signed-off-by: Fernando Rijo Cedeno <[email protected]>
@zFernand0
Copy link
Member

zFernand0 commented Mar 6, 2025

I'm trying to keep track of the test in the hopes that it would give us some perspective.

Nested if approach:
80bef2d
image

Cleaner switch approach:
c9e76a5
image

Aftr merging master (adf4304)
image


Personally, switch > if for complex stuff like this 😋

@ATorrise ATorrise requested a review from zFernand0 March 10, 2025 20:47
@ATorrise ATorrise marked this pull request as draft March 12, 2025 15:39
@ATorrise
Copy link
Contributor Author

momentarily moving this to draft status for a few hours to make a significant change based on sonar cloud's good suggestion

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

Add support for responseTimeout to other MVS and USS SDK functions
6 participants